Fix macOS recording duration when final frames stop updating#685
Open
kax168 wants to merge 1 commit into
Open
Conversation
Contributor
📝 WalkthroughWalkthrough
ChangesPause-aware video end padding in ScreenCaptureKitRecorder
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/native/ScreenCaptureKitRecorder.swift`:
- Around line 485-509: The appendFinalVideoFrameIfNeeded function appends a
padding frame whenever lastSampleBuffer exists, but should only append when the
video actually ends early relative to the capture. When captureEndTime is less
than or equal to videoEndTime, the targetPresentationTime becomes equal to
videoEndTime due to the max() calculation, resulting in an unnecessary extra
frame. Add a condition before creating and appending the final sample buffer to
check that captureEndTime is actually greater than videoEndTime, ensuring
padding is only added when needed to fill the gap between video and capture end
times.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: be701247-bcf3-4ba2-be03-a5454dd2ace7
📒 Files selected for processing (1)
electron/native/ScreenCaptureKitRecorder.swift
Comment on lines
+485
to
+509
| private func appendFinalVideoFrameIfNeeded(videoEndTime: CMTime, captureEndTime: CMTime) -> CMTime { | ||
| guard let originalBuffer = lastSampleBuffer, let videoInput else { | ||
| return videoEndTime | ||
| } | ||
|
|
||
| let duration = frameDuration(for: originalBuffer) | ||
| let minimumAdditionalTime = videoEndTime | ||
| let targetPresentationTime = max( | ||
| minimumAdditionalTime, | ||
| captureEndTime - duration | ||
| ) | ||
| let timing = CMSampleTimingInfo( | ||
| duration: duration, | ||
| presentationTimeStamp: targetPresentationTime, | ||
| decodeTimeStamp: originalBuffer.decodeTimeStamp | ||
| ) | ||
| if let finalSampleBuffer = try? CMSampleBuffer(copying: originalBuffer, withNewTiming: [timing]), | ||
| videoInput.append(finalSampleBuffer) { | ||
| lastVideoPresentationTime = targetPresentationTime | ||
| lastVideoDuration = duration | ||
| return targetPresentationTime + duration | ||
| } | ||
|
|
||
| return videoEndTime | ||
| } |
Contributor
There was a problem hiding this comment.
Only append the padding frame when the video actually ends early.
As written, this appends whenever lastSampleBuffer exists. When captureEndTime <= videoEndTime, targetPresentationTime becomes videoEndTime, so the returned end time is extended by one extra frame even though no padding was needed.
Proposed fix
private func appendFinalVideoFrameIfNeeded(videoEndTime: CMTime, captureEndTime: CMTime) -> CMTime {
guard let originalBuffer = lastSampleBuffer, let videoInput else {
return videoEndTime
}
+
+ guard CMTimeCompare(captureEndTime, videoEndTime) > 0 else {
+ return videoEndTime
+ }
let duration = frameDuration(for: originalBuffer)
let minimumAdditionalTime = videoEndTime
let targetPresentationTime = max(
minimumAdditionalTime,
captureEndTime - duration📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private func appendFinalVideoFrameIfNeeded(videoEndTime: CMTime, captureEndTime: CMTime) -> CMTime { | |
| guard let originalBuffer = lastSampleBuffer, let videoInput else { | |
| return videoEndTime | |
| } | |
| let duration = frameDuration(for: originalBuffer) | |
| let minimumAdditionalTime = videoEndTime | |
| let targetPresentationTime = max( | |
| minimumAdditionalTime, | |
| captureEndTime - duration | |
| ) | |
| let timing = CMSampleTimingInfo( | |
| duration: duration, | |
| presentationTimeStamp: targetPresentationTime, | |
| decodeTimeStamp: originalBuffer.decodeTimeStamp | |
| ) | |
| if let finalSampleBuffer = try? CMSampleBuffer(copying: originalBuffer, withNewTiming: [timing]), | |
| videoInput.append(finalSampleBuffer) { | |
| lastVideoPresentationTime = targetPresentationTime | |
| lastVideoDuration = duration | |
| return targetPresentationTime + duration | |
| } | |
| return videoEndTime | |
| } | |
| private func appendFinalVideoFrameIfNeeded(videoEndTime: CMTime, captureEndTime: CMTime) -> CMTime { | |
| guard let originalBuffer = lastSampleBuffer, let videoInput else { | |
| return videoEndTime | |
| } | |
| guard CMTimeCompare(captureEndTime, videoEndTime) > 0 else { | |
| return videoEndTime | |
| } | |
| let duration = frameDuration(for: originalBuffer) | |
| let minimumAdditionalTime = videoEndTime | |
| let targetPresentationTime = max( | |
| minimumAdditionalTime, | |
| captureEndTime - duration | |
| ) | |
| let timing = CMSampleTimingInfo( | |
| duration: duration, | |
| presentationTimeStamp: targetPresentationTime, | |
| decodeTimeStamp: originalBuffer.decodeTimeStamp | |
| ) | |
| if let finalSampleBuffer = try? CMSampleBuffer(copying: originalBuffer, withNewTiming: [timing]), | |
| videoInput.append(finalSampleBuffer) { | |
| lastVideoPresentationTime = targetPresentationTime | |
| lastVideoDuration = duration | |
| return targetPresentationTime + duration | |
| } | |
| return videoEndTime | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@electron/native/ScreenCaptureKitRecorder.swift` around lines 485 - 509, The
appendFinalVideoFrameIfNeeded function appends a padding frame whenever
lastSampleBuffer exists, but should only append when the video actually ends
early relative to the capture. When captureEndTime is less than or equal to
videoEndTime, the targetPresentationTime becomes equal to videoEndTime due to
the max() calculation, resulting in an unnecessary extra frame. Add a condition
before creating and appending the final sample buffer to check that
captureEndTime is actually greater than videoEndTime, ensuring padding is only
added when needed to fill the gap between video and capture end times.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Background
On macOS, ScreenCaptureKit may stop delivering new screen samples when the captured display/window is visually static. Recordly hides the native cursor and records cursor telemetry separately, so a session can continue collecting cursor data while the video track remains stuck at the last screen sample timestamp. The old finalization path appended only one extra frame, so the resulting MP4 duration could be much shorter than the real recording session.
Validation
npm run build:native-helpersNo user recording paths, filenames, or private recording content are included in this PR.
Summary by CodeRabbit
Release Notes